Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: less confusing permit2 flow #8508

Merged
merged 12 commits into from
Jan 9, 2025
Merged

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Jan 8, 2025

Description

This makes things less confusing for users and eng. alike, by clearly separating Permit2 contract allowance step and the actual Permit2 EIP-712 signing.

In effect:

  • Splits copies (titles, tooltips etc) and makes them clearer as to what's happening
  • Ensure both steps are clearly handled at runtime

Note, new copies are not yet approved by @shapeshift/product.

Issue (if applicable)

Risk

High Risk PRs Require 2 approvals

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Low, though for paranoia's sake, test this both with the old and new swapper flow and ensure things look good, see testing below.

Testing

Engineering

Test the three ecases below with both old and new swapper flow.

0x trade - No Permit2 allowance granted for asset

  • Infinite permit2 allowance grant step should be triggered, with sane looking copies that clearly explain what this is about
  • Permit2 message signing step should be triggered, with sane looking copies as well that clearly explain the diff. between this and the previous step

0x trade - Infinite Permit2 allowance already granted for asset

  • Ensure you have Infinite permit2 allowance already for that asset. You can check it on revoke.cash and confirm Permit2 has unlimited allowance for the asset you're selling. If you don't have it already granted, you can do so by starting the 0x swap flow, grant unlimited allowance, and hard refresh the app
  • 0x swapper should not trigger another Permit2 infinite allowance since already granted
  • 0x swapper should however trigger a Permit2 message signing, with copies looking sane

Non-0x-trade

  • Allowance stages/behaviour should look the same as before.

Operations

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)

Follow the same steps as eng, but only with the old swapper flow (the one you can see in prodish envs e.g release).
New swapper flow is still under flag and doesn't require ops testing.

Screenshots (if applicable)

New trade flow

  • 0x trade - No Permit2 allowance granted for asset

https://jam.dev/c/e250753f-03fb-401c-b354-26c7736aa382

  • 0x trade - Permit2 allowance granted for asset

https://jam.dev/c/79ac916c-23be-44aa-b36a-af6fe5035571

  • Good ol' allowances

https://jam.dev/c/d77ec8f2-11a6-4582-bcc9-6a28a32d4fe9

Old trade flow

  • 0x trade - No Permit2 allowance granted for asset

https://jam.dev/c/2cda5f82-4055-4665-afe0-a390ea87e9af

  • 0x trade - Permit2 allowance granted for asset

https://jam.dev/c/bc78efca-1b02-4873-a0be-c08c8ec74dd9

  • Good ol' allowances

https://jam.dev/c/9afe5f79-1295-46e5-9117-3450b67f3dc2

@gomesalexandre gomesalexandre marked this pull request as ready for review January 8, 2025 12:00
@gomesalexandre gomesalexandre requested a review from a team as a code owner January 8, 2025 12:00
Copy link
Member

@woodenfurniture woodenfurniture left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One Q but LGTM

@gomesalexandre gomesalexandre enabled auto-merge (squash) January 9, 2025 06:48
@gomesalexandre gomesalexandre merged commit f72d9ef into develop Jan 9, 2025
6 checks passed
@gomesalexandre gomesalexandre deleted the feat_permit2_improve branch January 9, 2025 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Less confusing Permit2 copies
2 participants